ci: force rustup proxy on PATH for macos-14 x86_64 builds#1151
Conversation
macos-14 ARM runners intermittently expose a stale cargo binary that resolves to rustup-init, breaking napi-rs `cargo metadata` calls during the x86_64-apple-darwin build. Add a verification step after dtolnay/rust-toolchain that re-prepends $HOME/.cargo/bin and runs `cargo --version` so the failure surfaces immediately rather than deep inside napi build. Fixes #1136
|
Claude finished @carlos-alm's task —— View job PR Review CompleteRoot Cause AnalysisProblem Being Solved: macos-14 ARM runners intermittently expose a stale Why This Approach: The fix adds explicit PATH management after Risk Assessment: Very low risk. Changes are minimal, targeted, and properly conditional. Only affects macOS jobs and adds early validation without changing core build logic. Backlog Compliance
Code Quality Assessment✅ Strengths:
📍 Technical Details:
Critical ConcernsNone identified. This is a well-targeted infrastructure fix. Final Recommendation
Recommendation: Approve and merge. Consider monitoring CI logs to confirm the fix resolves the intermittent failures on macos-14 runners. |
Greptile SummaryThis PR adds a defensive PATH-fixup step to both
Confidence Score: 5/5Safe to merge — the change is a targeted, read-only PATH fixup that cannot corrupt state and fails loudly if the cargo binaries are missing. Both workflow files receive an identical, well-scoped step: it prepends the rustup proxy directory to PATH for downstream steps and uses absolute-path invocations for immediate health-checking. The $GITHUB_PATH mechanism is used correctly (it takes effect in the next step, but verification intentionally uses absolute paths), the runner.os == 'macOS' guard is appropriate, and the step is positioned correctly after dtolnay/rust-toolchain. No existing steps are modified. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Setup Rust\ndtolnay/rust-toolchain] --> B{runner.os == 'macOS'?}
B -- Yes --> C[Verify cargo resolves to rustup proxy\nmacOS shim workaround]
C --> C1["echo $HOME/.cargo/bin >> $GITHUB_PATH\n(prepend to PATH for subsequent steps)"]
C1 --> C2["$HOME/.cargo/bin/cargo --version\n$HOME/.cargo/bin/rustc --version\n(fast-fail health check)"]
C2 -- Fail --> FAIL[❌ Build fails immediately\nshim regression surfaced early]
C2 -- Pass --> D
B -- No --> D[Rust cache\nSwatinem/rust-cache]
D --> E[Cross-compilation setup\nif needed]
E --> F[napi build --release --target ...]
F --> G[Upload artifact]
Reviews (9): Last reviewed commit: "Merge branch 'main' into fix/1136-cargo-..." | Re-trigger Greptile |
| targets: ${{ matrix.target }} | ||
|
|
||
| - name: Verify cargo resolves to rustup proxy (macos-14 shim workaround) | ||
| if: runner.os == 'macOS' |
There was a problem hiding this comment.
Condition covers all macOS runners, not just
macos-14
runner.os == 'macOS' also matches the macos-latest / aarch64-apple-darwin matrix entry, so the PATH fixup runs for the native ARM build too. This is harmless and arguably a safety net, but the step name ("macos-14 shim workaround") and the PR description ("specifically for macos-14") are slightly misleading. If the intent is to guard only the known-broken matrix entry you could use matrix.os == 'macos-14' instead. The same applies to publish.yml.
There was a problem hiding this comment.
Addressed in 6680ffe — renamed the step to "macOS shim workaround" and updated the comment to note that the workaround intentionally applies to all macOS matrix entries. Issue #1136 also recorded an aarch64-apple-darwin occurrence (PR #1127 run), so keeping the broader runner.os == 'macOS' guard is the right call as a safety net rather than narrowing to matrix.os == 'macos-14'. The step/comment wording now matches that intent.
Greptile noted that the step name ("macos-14 shim workaround") and PR description suggested the fix only targets macos-14, while runner.os == 'macOS' also covers macos-latest/aarch64-native. Issue #1136 also recorded an aarch64-apple-darwin failure, so keeping the broader guard is correct. Update the step name and comment to reflect that the workaround intentionally applies to all macOS matrix entries as a safety net.
Greptile noted that the step name ("macos-14 shim workaround") and PR
description suggested the fix only targets macos-14, while
runner.os == 'macOS' also covers macos-latest/aarch64-native. Issue
#1136 also recorded an aarch64-apple-darwin failure, so keeping the
broader guard is correct. Update the step name and comment to reflect
that the workaround intentionally applies to all macOS matrix entries
as a safety net.
6680ffe to
4103161
Compare
Summary
cargoon PATH that resolves torustup-init, breaking napi-rscargo metadatacalls during theBuild x86_64-apple-darwinjob (ci: Build x86_64-apple-darwin sporadically fails — cargo resolves to rustup-init on macos-14 #1136).dtolnay/rust-toolchainthat re-prepends$HOME/.cargo/binto$GITHUB_PATHand runscargo --version/rustc --versionso any future shim regressions fail fast and obviously instead of deep insidenapi build.build-native.ymlandpublish.yml(the matrix entry is duplicated across the two workflows).Test plan
Build Nativeagainst this branch and confirmBuild x86_64-apple-darwinsucceeds.Verify cargo resolves to rustup proxystep runs only on macOS jobs and is skipped on Linux/Windows.Closes #1136